-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Workflow default dialog alert #4100
Conversation
de248cc
to
79a8bee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few comments about the logic and answer your question about behavior inline.
For the test, I'm not sure which conditional you're referring to, can you be more specific?
|
||
const WorkflowDefaultDialog = ({ onSuccess }) => ( | ||
<div> | ||
<Translate content="workflowDefaultDialog.text" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this needs to be translated. The admin pages are only for Zooniverse team members.
app/pages/admin/project-status.jsx
Outdated
@@ -25,7 +27,8 @@ class ProjectStatus extends Component { | |||
project: null, | |||
error: null, | |||
usedWorkflowLevels: [], | |||
workflows: [] | |||
workflows: [], | |||
workflowSetting: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to be used somewhere? I don't see it used in the code below.
app/pages/admin/project-status.jsx
Outdated
<WorkflowDefaultDialog closeButton={true} required={true} /> | ||
) | ||
.then(() => { | ||
Promise.all(promises) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to your question in the PR:
if a user clicks the background or presses escape, the workflow retains its default status, but workflow.active still gets toggled (ie, it is still possible to have a default workflow that is inactive). Not sure if this is the desired UX, would appreciate input.
My suggestion to use Promise.all()
was for the project update and the workflow update. If a workflow is active and the project default, we want to prompt the admin user asking to confirm the switch and if they confirm, then remove that workflow as the default and make it inactive. If they cancel, then no action is taken.
At this point, you shouldn't first need to do a get request for the project and workflows again as you should already have them since they are called in componentDidMount
. The get request for the project and workflows should be after the successful Promise.all()
. What's defined in componentDidMount
could be moved into a class method to be reused here as well as in componentDidMount
.
app/pages/admin/project-status.jsx
Outdated
.then(() => { | ||
Promise.all(promises) | ||
.then(() => { | ||
if (!this.state.error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is an error, then the promise is rejected, which is what catch()
is for.
app/pages/admin/project-status.jsx
Outdated
return workflow.update({ 'active': checked }).save() | ||
.then(() => this.getWorkflows()) | ||
.catch(error => this.setState({ error })) | ||
workflow.update({ active: checked }).save(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also related to the question in the PR: if (defaultWorkflowId !== workflow.id)
then update the workflow. We don't want to always update the active state for the workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more organizational comment: The components
folder is meant for reusable components. The new dialog is built for use with the admin pages and won't be reused so it should be put into the admin folder.
Thanks :) Regarding the tests, do I need to create a spec file for ProjectStatus and then check whether WorkflowDefaultDialog component renders as expected, i.e. simulate a click on WorkflowToggle, set state so that |
- Remove Translation component and unused code. - Use Promise.all for update requests. - Add class method for wrkflow & proj GET req's. - Move WorkflowDefaultDialog to admin folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few more minor comments inline.
app/pages/admin/project-status.jsx
Outdated
const defaultWorkflowId = this.state.project.configuration.default_workflow; | ||
|
||
if (defaultWorkflowId === workflow.id && workflow.active) { | ||
const promises = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this variable in the then
block of the Dialog alert promise.
<div> | ||
You are about to make the default workflow inactive, | ||
which will remove the default setting from this workflow. | ||
The default workflow can be set in the workflows page of the project builder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be specific that the option is on the workflow edit page.
<div> | ||
<div> | ||
You are about to make the default workflow inactive, | ||
which will remove the default setting from this workflow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It removes the default setting from the project.
assert.equal(WorkflowDefaultDialogContainer.length, 1); | ||
}); | ||
|
||
it('renders dialog text', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there isn't a React component we're testing the presence of anymore. This is just testing that JSX works, and this isn't necessary for us to test again.
app/pages/admin/project-status.jsx
Outdated
.then(() => this.getWorkflows()) | ||
.catch(error => this.setState({ error })) | ||
if (defaultWorkflowId !== workflow.id) { | ||
workflow.update({ active: checked }).save(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should still have a catch
- Move promises object inside .then() block of Dialog.alert. - Add catch block for updating workflow status request. - Update modal text. - Remove test for modal text content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never been a fan of the modal-form's design to co-opt native browser form functionality and use it with in a Promise. It was designed to reject the promise on cancellation, but that means we can't really catch actual errors in a useful way. How about instead of using the alert built into modal-form, we just manually handle the open/close state? There should be an example in the modal-form readme on how to do this.
RFC: I added an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few more comments and rethinking how we handle the project and workflow update order. Thanks!
app/pages/admin/project-status.jsx
Outdated
@@ -10,6 +10,7 @@ import VersionList from './project-status/version-list'; | |||
import ExperimentalFeatures from './project-status/experimental-features'; | |||
import Toggle from './project-status/toggle'; | |||
import RedirectToggle from './project-status/redirect-toggle'; | |||
import WorkflowDefaultDialog from './workflow-default-dialog' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking, but this is missing a semi-colon.
app/pages/admin/project-status.jsx
Outdated
@@ -164,6 +210,8 @@ class ProjectStatus extends Component { | |||
<div className="project-status__section"> | |||
<h4>Workflow Settings</h4> | |||
<small>The workflow level dropdown is for the workflow assignment experimental feature.</small> | |||
<br /> | |||
<small>An asterisk (*) denotes a default workflow.</small> | |||
{this.renderError()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a React error being thrown, Error: Objects are not valid as a React child
when an error is set to the component because this is expecting a this.state.error
to be a string. We changed how errors are handled in the javascript client to send along the full error object instead, so how the error renders should be modified too. Perhaps the status and message together?:
{`Error ${this.state.error.status}: ${this.state.error.message}`}
app/pages/admin/project-status.jsx
Outdated
this.state.project.update({ 'configuration.default_workflow': undefined }).save() | ||
]; | ||
|
||
Promise.all(promises) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm rethinking the use of Promise.all()
since the possibility of a failure condition on the project PUT. We don't want the workflow changed if updating the default workflow fails. It made sense initially to have a single then()
which Promise.all()
allows us, but I'm thinking this will be safer:
this.state.project.update({ 'configuration.default_workflow': undefined }).save()
.then(() -> {
workflow.update({ active: false }).save()
.then(this.getProjectAndWorkflows() // Only get project and workflows after successful project and workflow PUT
).catch(// its own catch);
}).catch(// its own catch)
.then(() => {
this.setState({ dialogIsOpen: false });
})
This is just a rough idea without testing, so it might need tweaks.
app/pages/admin/project-status.jsx
Outdated
this.setState({ dialogIsOpen: true }); | ||
} | ||
|
||
if (defaultWorkflowId !== workflow.id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and looked at the lab code and if a project isn't live, then the default workflow button can still be enabled for inactive workflows. I think this should change, but to make sure we still allow admins to toggle the checkbox as needed, this conditional needs modification:
((defaultWorkflowId !== workflow.id) || (defaultWorkflowId === workflow.id && !workflow.active))
which will remove the default workflow setting from this project. | ||
The default workflow can be set in the edit workflows page of the project builder. | ||
</div> | ||
<br /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed seeing the <br />
last review. If the goal is to have a bit of space, then perhaps use a <p>
around the text instead of a<div>
and a <br />
. Paragraphs are also block-level and browsers default to given them margins too.
}); | ||
|
||
it('calls the onSuccess handler', () => { | ||
wrapper.find('button#workflowDefaultDialogSuccess').simulate('click'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not adding CSS ids or classes to elements just for the purposes of testing. Since there are only two buttons using enzyme's first()
or last()
is fine or at()
- Refactor handleDialogSuccess to remove Promise.all() - Update error rendering to display status and msg from error obj. - Update modal UI to separate buttons from text with <p>. - Remove button ids, target via first() and last() for tests. - Fix linter errors.
I'm seeing some weird behavior when toggling a default workflow from active to inactive - when there is more than one workflow, it toggles the last workflow, rather than the one that was the default. The default setting is getting removed though. Wanted to check and see if you can reproduce this issue though, in case it's a cache bug again (I tried on Firefox though, got same result, so I don't think it is). Also, on my local, the workflow being passed into |
Got a working solution to this ^^ issue (with a little input from Mark - thx!) - I think what was happening was that because To fix this, I stored the default workflow id in state, and no longer pass it in the onSuccess event. In Happy to change or consider a different solution of course :) |
8f7f9c3
to
52c8723
Compare
app/pages/admin/project-status.jsx
Outdated
@@ -85,17 +86,21 @@ class ProjectStatus extends Component { | |||
this.setState({ dialogIsOpen: false }); | |||
} | |||
|
|||
handleDialogSuccess(workflow) { | |||
handleDialogSuccess() { | |||
const defaultWorkflow = this.state.workflows.filter(workflow => workflow.id === this.state.defaultWorkflowId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to call this variable workflow
, but that raised a linter error ('workflow' is already declared in the upper scope
), so I went with defaultWorkflow, which I guess is more explicit anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of array.filter()
, however, we can sidestep storing the default workflow id in the component state since it's already stored in the project resource:
const defaultWorkflow = this.state.workflows.filter(workflow => workflow.id === this.state.project.configuration.default_workflow);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just have a minor change request: good idea to use the filter, but storing the default workflow id in the state is redundant.
app/pages/admin/project-status.jsx
Outdated
@@ -85,17 +86,21 @@ class ProjectStatus extends Component { | |||
this.setState({ dialogIsOpen: false }); | |||
} | |||
|
|||
handleDialogSuccess(workflow) { | |||
handleDialogSuccess() { | |||
const defaultWorkflow = this.state.workflows.filter(workflow => workflow.id === this.state.defaultWorkflowId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of array.filter()
, however, we can sidestep storing the default workflow id in the component state since it's already stored in the project resource:
const defaultWorkflow = this.state.workflows.filter(workflow => workflow.id === this.state.project.configuration.default_workflow);
app/pages/admin/project-status.jsx
Outdated
@@ -106,7 +111,10 @@ class ProjectStatus extends Component { | |||
const defaultWorkflowId = this.state.project.configuration.default_workflow; | |||
|
|||
if (defaultWorkflowId === workflow.id && workflow.active) { | |||
this.setState({ dialogIsOpen: true }); | |||
this.setState({ | |||
defaultWorkflowId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant with the default_workflow
already a property on the project resource which is already stored in the component state.
Describe your changes
workflow.active
still gets toggled (ie, it is still possible to have a default workflow that is inactive). Not sure if this is the desired UX, would appreciate input.Content of WorkflowDefaultDialog.Review Checklist
rm -rf node_modules/ && npm install
and app works as expected?Optional
ChangeListener
orPromiseRenderer
components with code that updates component state?